-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Plugins #81
base: main
Are you sure you want to change the base?
Add Plugins #81
Conversation
proto/plugin/plugin.proto
Outdated
syntax = "proto3"; | ||
|
||
package helperPlugin; | ||
option go_package = "../../pkg/helperPlugin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this package must be in the same folder we are
proto/plugin/plugin.proto
Outdated
} | ||
|
||
service SpiffeHelper { | ||
rpc PostConfigs(ConfigsRequest) returns (Empty) {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are those configs? only paths?
I feels like this plugin ends is to notify "something" about there are updated on bundles,
so we must know what is updated, and what to do based on each plugin config,
So the Service may be something like Notifier
, and your PostConfig, may be Notify
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, most of the configs are paths to certs. The current approach is to execute the plugins on each cert update (and let them do what they want with those certs), but yes, it is possible to keep them running and send a notification on updates.
proto/plugin/plugin.proto
Outdated
message Empty {} | ||
|
||
message ConfigsRequest { | ||
map<string,string> configs = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what this configs are?
What data is the Plugin getting in order to understand where to find certs, or what kind of cert was received?
and how is each plugin configured? is it possible to provide data to this plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configs contains most of the spiffe-helper .conf fields and the plugin specific configurations
Hi @faisal-memon, it would be great if you can take a look to this proof of concept. It is related to 65. |
pkg/notifier/notifier.proto
Outdated
} | ||
|
||
service Notifier { | ||
rpc LoadConfigs(ConfigsRequest) returns (Empty) {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm generally we create messages for request or response, no matter if they are empty,
but when we want to add fields in a future, it keep working without back compatibility issues.
So I suggest to add messages for both requests and response
pkg/notifier/shared.go
Outdated
grpc "google.golang.org/grpc" | ||
) | ||
|
||
type Notifier interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are not you using notifier.NotifierServer? (the one autogenerated from proto)
Whats the status of this? I think it is useful to the helm-charts-hardened project. |
Hi @kfox1111 I'm going to keep working on this |
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
Signed-off-by: JU4N98 <[email protected]>
0cbf201
to
fb121c7
Compare
Signed-off-by: JU4N98 <[email protected]>
A thought.... This plugin support requires spire-helper to execute the plugin. In a traditional environment, that works very well. In a containerized environment such as Kubernetes though, that would require spire-helper along with the plugins to be in the same container, which is more difficult as generally those come from different sources. In that environment, its much easier for them to be different containers, have Kubernetes exec the containers, and share between them with unix sockets. Could this pr be updated to support that mode of operation too? |
I think something like that is out of scope for this PR, the main idea was to add support for plugins that runs alongside the helper in the same container. But is the initial approach, maybe that could be added in other PR as other kind of plugins. |
Signed-off-by: JU4N98 <[email protected]>
Probably ok to do in a followon, but maybe make sure the api leaves room so that it can be done via socket instead of exec without having to move around config items? |
Looks like that could be done by adding a 'type' field or something to the plugin to specify that path is an executable (default) vs named socket, so, I think we're good already? |
Yes, a field like type should be enough to treat the differently. I think isn't necessary to add anything since I'm parsing each plugin into a map of string. |
Signed-off-by: JU4N98 <[email protected]>
Changes proposal:
Adds support for plugins according with issue #65, on each X.509 SVID, JWT SVID or JWT Bundle update, the helper will notify this change to all the loaded plugins. The block plugins is added to the config file where the configuration of every plugin is specified. For example:
In this example a plugin called simple-plugin is going to be loaded into the spiffe-helper, there are two mandatory fields:
But it is also possible to specify other string configurations for each plugin, in this case the simple-plugin has three: from, to and message. All the configurations will be shared on the plugin load.